-
-
Notifications
You must be signed in to change notification settings - Fork 509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for google-style docstrings #963
Conversation
e55e3ad
to
118a13b
Compare
All CI-tests are now passing on this branch (except for pypy, which I believe is failing on master as well). |
@davidhalter If I fix the conflicts can this PR get merged? PyTorch currently uses Google style docstrings and it would be very useful for jedi to work with that library. |
9627274
to
748ac18
Compare
Conflicts have been fixed. |
Is this PR dead or was is implemented somewhere else? |
I still want the feature, but I haven't tried to get it merged in awhile. If @davidhalter is interested in merging the feature, I could probably spruce this up to be compatible with jedi-master. The CI tests pass, not sure how important -0.009% of coverage is. |
Yeah sorry. It's really my bad that I never got back to it. There's just too many things that are keeping me busy. This branch is well written and everything, so no offense @Erotemic, I feel like you tackled this problem well and found a good solution. All in all I won't merge for a couple of reasons:
All in all there's just not much that speaks for merging it. I struggle to find the upsides, except for the well written code. I guess that's also why I couldn't bring myself to close it sooner. Please keep in mind I do this all in my spare time and these additional 600 lines cost me 10+ hours a year (just for maintenance and bug reports). If you sum that up for all the feature/pull requests I get, I just have to cut my losses and decide what's actually worth it. So sorry for not being clearer a few years ago. |
While I'm disappointed, I understand the reality and cost of maintaining open source projects on your own time. While I want to respond to some points, I ultimately respect your decision.
There is certainly a lot of logic (300 lines) in However, if you don't count that file, there are only 49 lines of code that modify Jedi (23 of which are doctests and documentation). Those 26 lines of logic simply follow existing patterns used to support the other Epydoc and Numpydoc docstring styles. The remaining 202 lines of code are for tests, which again mirror the existing tests for other docstring styles.
I'm not sure where you're getting that mostly commercial observation. Its certainly less used than other styles, but it has been catching on. Large open-source projects like pytorch and many academic deep-learning research repos use google style docstrings.
I wish I could. Unfortunately, myself and many others are stuck writing code that has to be 2/3 compatible for the foreseeable future. Even so, type annotations have are considerably less flexible than docstring types because they need to be defined in-code. Doing anything semi-complex with them can slow down the startup time of an application, and that's a big downside. While type annotations are great, they aren't a perfect solution. As a closing thought, would you be more inclined to consider merging if I separated out the |
While this PR hasn't received as many emojis as other PRs, five emojis is still a non-zero amount of emojis. For those people, I want to note that I'm going to look into an alternative solution to this problem. Because the actual logic required to implement this capability into jedi is relatively small, (23 lines), I can probably create some logic that monkey-patches the |
@Erotemic I just want to say that I also have to maintain 2/3 compatible code and I am sure there's a lot of devs like us. I don't know if I can help you with development but please update here and ask if you will need a tester or any other help. Edit: I have just added an emoji reaction to the PR 🥇 |
I've made a new PR for my vimtk library, which adds a function that monkey patches jedi with the contents of this PR. If you use vim, then once I accept that vimtk-PR and release the new version (in a few days), you can install if has('python3')
command! -nargs=1 Python2or3 python3 <args>
elseif has('python')
command! -nargs=1 Python2or3 python <args>
else
echo "Error: Requires Vim compiled with +python or +python3"
finish
endif
Python2or3 << EOF
from vimtk import jedi_monkeypatch
jedi_monkeypatch.apply_monkey_patch_jedi()
EOF If you don't use vim, or don't want to install https://github.com/Erotemic/vimtk/pull/2/files#diff-17489446620e53eb9a11adc3185cf8a1 Its fairly simple. It checks the jedi version to ensure the monkey patch still works, and then it replaces the relevant jedi functions that handle docstrings with modified ones that also handle google-style docstrings. I left in debugging statements as I've given up on doing things the clean way for this feature. |
@Erotemic The problem is |
This a much reduced version of PR #796 with the goal of adding support for using Google style docstrings for paramter / return type inference.
Thanks to @davidhalter for the comments on the original PR. Also, thanks so much to the authors of #868 for working in the fixes I made when I originally tried to implement this functionality. Having those changes in made this a much easier job the second time around. That being said, this PR is much more self-contained.
The main changes here are:
jedi/evaluate/docscape_google.py
for parsing/scraping google-style docstringsjedi/evaluate/docstrings.py
Looking at the diff, I see my editor took out a bunch of trailing whitespace. If you want me to undo that I can.